-
Notifications
You must be signed in to change notification settings - Fork 34
Fix unreliable timeout #376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for putting up this potential fix! Once #378 lands (fixes some CI issues), would you mind updating this branch so that we can run the conformance test suite against it on CI? Looks like you'll also have to update your commit to pass DCO |
5999bb7 to
c6b161e
Compare
Signed-off-by: Ludovic Ollagnier <[email protected]>
Signed-off-by: Ludovic Ollagnier <[email protected]>
c6b161e to
058dd7c
Compare
| } | ||
|
|
||
| self.closeConnection() | ||
| self.hasResponded = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we be consistent about setting hasResponded before closeConnection() calls?
| } | ||
|
|
||
| self.closeConnection() | ||
| self.hasResponded = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively to my previous comment, what do you think about moving self.hasResponded = true to closeConnection? Seems like that would simplify this a bit
|
CI checks look good! Left a couple comments. Thanks for submitting this! |
Fix: Notify caller when channel becomes inactive unexpectedly
Problem
When network connectivity is lost (e.g., 100% packet loss), requests hang indefinitely instead of timing out or returning an error. This is a regression
introduced in v1.1.0.
Fixes #361
Root Cause
When network drops, NIO's HTTP/2 layer detects the connection loss and triggers
channelInactivebefore theIdleStateHandlercan fire its timeout event.However,
channelInactiveonly closed the connection without notifying the caller:This worked in v1.0.4 because the retain cycle (fixed in #354) kept handlers alive longer, giving IdleStateHandler time to fire. With the fix, cleanup happens faster and
channelInactivewins the race—but doesn't notify the caller.Solution
hasRespondedflag to track whether the response callback has been invokedchannelInactiveto call the response callback with.unavailableerror if no response was received yetChanges
ConnectUnaryChannelHandler.swift: AddedhasRespondedflag and updatedchannelInactiveConnectStreamChannelHandler.swift: AddedhasRespondedflag and updatedchannelInactiveTesting
Tested with Network Link Conditioner at 100% packet loss: